Harden /slack/events and report real client IP to Sentry by irvingpop · Pull Request #466 · OperationCode/operationcode-pybot

irvingpop · 2026-11-07T15:52:08Z

Summary

Fixes the problems found while triaging Sentry issue 7822478670. In that issue, a curl client sent an empty POST to /slack/events, and the bot returned an unhandled 500.

client --> ALB (appends client IP to X-Forwarded-For) --> pybot
                                                           |
          before: request.json() raises -> 500 -> Sentry   |
          after:  400, no Sentry event                     v

400 for bad bodies. /slack/events now returns 400 when the body is not a JSON object (empty, invalid, or a list). Before, request.json() raised before the signature check, so any client could create a Sentry error.
No KeyError in url_verification. The handler now uses payload.get() for token and challenge.
Real client IP in Sentry. The aiohttp integration sets REMOTE_ADDR from request.remote, which is the ALB's private IP (172.20.x.x). A new before_send hook (also applied to transactions) replaces it with the IP that the ALB appended to X-Forwarded-For. It uses the rightmost entry, because the client can forge the entries to the left of it. SENTRY_TRUSTED_PROXY_HOPS sets the hop count (default 1). If PII settings removed REMOTE_ADDR, the hook does not add an IP.
- An aiohttp middleware cannot fix this, because Sentry captures the request before app middlewares run.
Dependency update (poetry update): sentry-sdk 2.73.0 to 2.74.0, zipcodes 3.0.1, coverage 7.17.2, ruff 0.17.4, pytest-asyncio 1.4.1.post1.

Testing

New tests: tests/unit/test_sentry_client_ip.py, and bad-body and url_verification tests in tests/endpoints/slack/test_slack_events.py.
pytest: 269 passed, 10 skipped. ruff check and ruff format --check are clean.
End-to-end check with the real Sentry SDK and a capture transport: the header X-Forwarded-For: 6.6.6.6, 203.0.113.7 gave REMOTE_ADDR: 203.0.113.7 in the event.

Fixes OPERATIONCODE-PYBOT-7S

🤖 Generated with Claude Code

- Return 400 for a /slack/events body that is not a JSON object. Before, json.loads raised and the handler returned an unhandled 500 (Sentry issue 7822478670, an empty curl POST). - Use payload.get() in the url_verification path so a payload with no token or challenge cannot raise KeyError. - Add a Sentry before_send hook that sets REMOTE_ADDR from the X-Forwarded-For entry the ALB appends. The aiohttp integration uses request.remote, so every event showed the ALB's private IP. Hop count is configurable with SENTRY_TRUSTED_PROXY_HOPS (default 1). - poetry update: sentry-sdk 2.74.0, zipcodes 3.0.1, coverage 7.17.2, ruff 0.17.4, pytest-asyncio 1.4.1.post1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions

Claude of Duty review, run 42388456227: 2 inline finding(s). Model output from untrusted input. Check it before you act on it. The gate result is in the summary comment.

github-actions · 2026-11-07T15:53:30Z

+    if not forwarded_for:
+        return event
+
+    hops = config("SENTRY_TRUSTED_PROXY_HOPS", default=1, cast=int)


low bug, confidence medium, pass review

Invalid SENTRY_TRUSTED_PROXY_HOPS raises inside before_send

If the env var is non-numeric, int() raises ValueError on every event that has REMOTE_ADDR and X-Forwarded-For. Sentry swallows hook exceptions and may drop the event or log errors, so a config typo silently loses events. The value should be parsed defensively with a fallback to 1.

Model output from untrusted input. Check it before you act on it.

Fixed in 4adf12d. Confirmed that Sentry drops the event when before_send raises (sentry_sdk/client.py). A value that is not a positive integer now logs a warning and falls back to 1. Tests cover abc, empty, 0, -2 and 1.5.

github-actions · 2026-11-07T15:53:30Z

 [[package]]
 name = "sentry-sdk"
-version = "2.73.0"
+version = "2.74.0"


low supply-chain, confidence low, pass supply-chain

sentry-sdk runtime dependency bumped to a release under 72 hours old

The lockfile moves sentry-sdk from 2.73.0 to 2.74.0, a release the prefilter flagged as published about 49 hours earlier. sentry-sdk is a main (runtime) dependency, and it loads and runs inside the bot, so a compromised release would run with the bot's environment and secrets. The package is from the official PyPI registry with a wheel and no install scripts, and nothing in the bundle shows tampering. The same freshness flag applies to coverage 7.17.2 and ruff 0.17.4, which are dev-only. A maintainer may want to let the release age or compare it with the upstream tag before merging.

Model output from untrusted input. Check it before you act on it.

github-actions · 2026-11-07T15:53:33Z

🔴 Hold deployment: Claude of Duty identified 4 items requiring attention

Mission: Review the code. Protect the branch. Deploy the future.

Caution

Recently published dependency and 2 other hard block(s) block this PR. No waiver can clear them.
Next step for the author: fix the cause named in the row, then push. The review runs again on the new commit.

Severity	Effect	Where	Finding	Found by
🚫 Hard block	Blocks, no waiver	`poetry.lock`	Recently published dependency: published 68.5 hours ago (under 72), in `poetry.lock`, coverage@7.17.2	File checks (code, no model)
🚫 Hard block	Blocks, no waiver	`poetry.lock`	Recently published dependency: published 45.5 hours ago (under 72), in `poetry.lock`, ruff@0.17.4	File checks (code, no model)
🚫 Hard block	Blocks, no waiver	`poetry.lock`	Recently published dependency: published 49.5 hours ago (under 72), in `poetry.lock`, sentry-sdk@2.74.0	File checks (code, no model)
🟡 Low	Does not block	`poetry.lock:1405`	Runtime dependency bumped to a release published ~49 hours earlier _{See the inline comment.}	Supply chain (low)

File checks	Review	Security	Supply chain	Dependency review	Coverage
🚫 3 hard blocks, 2 signals	✅ ran, 0 findings	✅ ran, 0 findings	✅ ran, 1 finding	✅ ok	✅ complete

_{Run 42390834691, attempt 1 for head a3d8673 into master. The required workflow in the merge box is the result that counts.}

Claude of Duty never approves a PR. A human reviewer gives the final approval.

📋 Full review record: reasons, checks, signals, finding details, model summaries, token usage

Reviewed snapshot

Head SHA	Merge base SHA	Base branch	Run
`a3d86731422ce6d81ae5080a09060467eb740bde`	`d098f14ddf08e750f317c77e62684eec40f40b2a`	master	42390834691, attempt 1

Reasons the gate failed

Reason	Waivable?	Detail
Hard block from the file checks	🚫 no	`fresh-publish in poetry.lock`

No waiver line is shown. At least one reason cannot be waived, so a waiver would not change the result.

Checks (code, no model)

Check	Result	Detail
Hard block	🚫 blocks	Recently published dependency: published 68.5 hours ago (under 72), in `poetry.lock`, coverage@7.17.2
Hard block	🚫 blocks	Recently published dependency: published 45.5 hours ago (under 72), in `poetry.lock`, ruff@0.17.4
Hard block	🚫 blocks	Recently published dependency: published 49.5 hours ago (under 72), in `poetry.lock`, sentry-sdk@2.74.0
Coverage	✅ complete	files in comparison 7, files collected 7, trees complete
Dependency changes	ℹ️ listed below
Dependency review job	✅ success

Signals (context only, never block)

ℹ️ Credential-like string: credential-assignment (value redacted), in tests/endpoints/slack/test_slack_events.py, lines 269
ℹ️ Credential-like string: credential-assignment (value redacted), in tests/unit/test_slack_signature.py, lines 12

Dependency changes

pypi coverage: 7.17.0 to 7.17.2, source registry pypi.org, in poetry.lock, published 2026-11-04T19:49:22.217221+00:00
pypi pytest-asyncio: 1.4.1 to 1.4.1.post1, source registry pypi.org, in poetry.lock, published 2026-11-01T18:26:02.168875+00:00
pypi ruff: 0.17.3 to 0.17.4, source registry pypi.org, in poetry.lock, published 2026-11-05T18:51:36.594688+00:00
pypi sentry-sdk: 2.73.0 to 2.74.0, source registry pypi.org, in poetry.lock, published 2026-11-05T14:50:25.767248+00:00
pypi zipcodes: 3.0.0 to 3.0.1, source registry pypi.org, in poetry.lock, published 2026-11-02T02:25:06.993493+00:00

Finding details

Model output from untrusted input. Check it before you act on it. Verified means only that the quoted text is at that place. It does not mean the claim is true.

Blocking is decided by code from category and severity, never by the model. A finding in a blocking category blocks only at high or critical severity.

Pass	Severity	Category	Confidence	Citation	Posted	Model title
Supply chain	low	supply chain	low	✅ checked	inline	Runtime dependency bumped to a release published ~49 hours earlier

The Review pass completed and reported no findings.

The Security pass completed and reported no findings.

Model summaries

Model output from untrusted input. Check it before you act on it. Verified means only that the quoted text is at that place. It does not mean the claim is true.

Pass	Status	Model	Summary
Review	✅ ok	claude-sonnet-5-5	This PR adds 400 handling for bad /slack/events bodies, tolerates missing Slack signature headers, uses .get() in url_verification, and adds a Sentry before_send hook that takes the real client IP from the rightmost X-Forwarded-For entry. It also bumps dependency versions in poetry.lock. I found no clear correctness bugs in the changed code. The unparenthesized `except TypeError, ValueError:` is valid on the project's Python 3.14 target, and the invalid-hops fallback behaves as intended.
Security	✅ ok	claude-sonnet-5-5	The PR makes /slack/events return 400 for non-JSON or non-object bodies, uses safer .get() lookups in url_verification, and makes Slack signature validation raise InvalidSlackSignature instead of KeyError or ValueError when headers are missing or malformed. It also adds a Sentry before_send hook that takes the client IP from the rightmost X-Forwarded-For entry, plus a dependency bump and tests. I found no security regressions. The signature and token checks are equal or stricter than before: the empty-token case now fails closed, and the HMAC comparison still uses compare_digest. The Sentry IP rewrite only affects event metadata, and it relies on the app being reachable only through the trusted proxy. The test secrets are redacted placeholders.
Supply chain	✅ ok	claude-sonnet-5-5	This PR hardens the /slack/events endpoint, makes Slack signature validation tolerate missing or malformed headers, adds a Sentry before_send hook that takes the client IP from the right side of X-Forwarded-For, adds tests, and runs a blanket poetry update. I found no obfuscation, dynamic code execution, unexpected network calls, install-time scripts, CI changes, or text aimed at AI reviewers. The new code only reads one env var (SENTRY_TRUSTED_PROXY_HOPS) and edits Sentry events locally. The lockfile changes come from the official PyPI registry. The one concern is that the poetry update pulls in freshly published releases, including sentry-sdk, which ships in the runtime dependency group. The prefilter flagged three fresh-publish hard blocks.

Token usage

Pass	Input	Output	Cache write	Cache read	Attempts
Review	721	608	0	202630	1
Security	1047	491	0	202630	1
Supply chain	1239	1092	202630	0	1
Total	3007	2191	202630	405260	3

before_send ran int() on the env var. A bad value raised, and Sentry drops the event when before_send raises. Now a value that is not a positive integer logs a warning and uses 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions

Claude of Duty review, run 42389157366: 1 inline finding(s). Model output from untrusted input. Check it before you act on it. The gate result is in the summary comment.

github-actions · 2026-11-07T16:02:28Z

 [[package]]
 name = "sentry-sdk"
-version = "2.73.0"
+version = "2.74.0"


low supply-chain, confidence low, pass supply-chain

Runtime dependency sentry-sdk bumped to a release under 72h old

The lockfile moves sentry-sdk from 2.73.0 to 2.74.0, which the prefilter shows was published about 49 hours before the PR. The source is the official PyPI registry and the change is plausibly an ordinary `poetry update`. Even so, sentry-sdk runs in production with the app's DSN and request data, and a very fresh release has had little time for the community to spot a compromised or yanked upload. Coverage 7.17.2 and ruff 0.17.4 (dev-only) are also under 72 hours old. Consider waiting for the release to age or checking the release against upstream before merge. This is a precaution, not evidence of harm.

Model output from untrusted input. Check it before you act on it.

irvingpop · 2026-11-07T16:14:06Z

Missing or malformed Slack signature headers still give an unhandled exception (and a Sentry event) here, which is the same class of bug this PR sets out to fix.

validate_request_signature in pybot/_vendor/slack/sansio.py does int(headers["X-Slack-Request-Timestamp"]) (line 370) and headers["X-Slack-Signature"] (line 375). A missing header raises KeyError, and a non-numeric timestamp raises ValueError. Neither is caught:

pybot/_vendor/sirbot/plugins/slack/endpoints.py:33-40 (url_verification branch) catches only InvalidSlackSignature and InvalidTimestamp
endpoints.py:46-50 has the same gap for every other event type, through _validate_request (lines 172-177)

So with signing_secret set (the production config), curl -X POST -H 'Content-Type: application/json' -d '{}' <host>/slack/events should still end in a 500 and a Sentry error.

Two ways to fix it:

Make validate_request_signature raise InvalidSlackSignature when either header is missing or the timestamp is not an int. This fixes both call sites at once, so I'd lean this way.
Catch KeyError and ValueError at both call sites and return 401.

A test that posts to /slack/events with no Slack headers (and one with X-Slack-Request-Timestamp: abc) would lock it in.

validate_request_signature read X-Slack-Request-Timestamp and X-Slack-Signature with headers[...] and int(). A missing header raised KeyError, and a non-numeric timestamp raised ValueError. Callers catch only InvalidSlackSignature and InvalidTimestamp, so a POST without Slack headers still gave an unhandled 500 and a Sentry error. Now both cases raise InvalidSlackSignature, so every caller returns its normal rejection status (401 for events, 500 for url_verification). Tests cover missing headers and a bad timestamp at the unit level and through /slack/events, and fail on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

irvingpop · 2026-11-07T16:18:36Z

Fixed in ee8800a with option 1. validate_request_signature now raises InvalidSlackSignature when X-Slack-Request-Timestamp or X-Slack-Signature is missing, or when the timestamp is not an integer. All callers already catch that exception. Events now return 401, and url_verification returns 500 as before, with no unhandled exception. New unit tests and /slack/events tests (no headers, X-Slack-Request-Timestamp: abc, missing signature with a current timestamp) fail on the old code. The url_verification tests assert that aiohttp logged no unhandled error, because a 500 status alone cannot tell the two cases apart. The change got a local review before this push. Suite: 291 passed, 10 skipped.

github-actions

Claude of Duty review, run 42390635874: 1 inline finding(s). Model output from untrusted input. Check it before you act on it. The gate result is in the summary comment.

github-actions · 2026-11-07T16:20:01Z

 [[package]]
 name = "sentry-sdk"
-version = "2.73.0"
+version = "2.74.0"


low supply-chain, confidence low, pass supply-chain

Runtime dependency sentry-sdk bumped to a release under 72 hours old

The lockfile moves sentry-sdk, a production dependency that runs in the bot process at runtime, to 2.74.0. The registry metadata shows it was published about 49 hours before review, which the prefilter flags as a fresh-publish hard block. coverage 7.17.2 and ruff 0.17.4 (dev-only, run in tests and CI) are in the same position. The source is the official PyPI registry, the package has no install scripts, and the PR gives a plausible reason (a general poetry update). Nothing in the bundle shows tampering. The risk is only that very new releases have had little time for community vetting or yanking. A maintainer may want to confirm the new hashes against the upstream releases or wait out the cooldown.

Model output from untrusted input. Check it before you act on it.

The url_verification test matched aiohttp's 'Error handling request' text. Check for an aiohttp.server record with exc_info instead, so a reworded message cannot hide an unhandled exception. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions

Claude of Duty review, run 42390834691: 1 inline finding(s). Model output from untrusted input. Check it before you act on it. The gate result is in the summary comment.

github-actions · 2026-11-07T16:22:15Z

 [[package]]
 name = "sentry-sdk"
-version = "2.73.0"
+version = "2.74.0"


low supply-chain, confidence low, pass supply-chain

Runtime dependency bumped to a release published ~49 hours earlier

sentry-sdk 2.74.0 is in the main (runtime) dependency group and was published about 49 hours before this review, which is under the 72-hour freshness threshold. coverage 7.17.2 and ruff 0.17.4 (both dev-only) are also under it. The registry is the official PyPI and nothing in the delta looks malicious. The bump comes from a blanket poetry update, not from anything the Sentry change needs. A brief hold, or pinning to the previous sentry-sdk release, would let any bad release be yanked before it reaches production.

Model output from untrusted input. Check it before you act on it.

github-actions Bot reviewed Nov 7, 2026

View reviewed changes

This was referenced Nov 7, 2026

Summary comment: clearer fresh-publish blocks, readable package names, no duplicate inline comments OperationCode/.github#32

Open

Review PRs larger than the size gate OperationCode/.github#29

Open

github-actions Bot reviewed Nov 7, 2026

View reviewed changes

irvingpop mentioned this pull request Nov 7, 2026

Docker hardening: non-root user, build attestations, and runtime lockdown #467

Open

6 tasks

irvingpop merged commit 3edff7a into master Nov 7, 2026
8 of 9 checks passed

irvingpop deleted the irving/202611-slack-events-hardening branch November 7, 2026 16:29

Conversation

irvingpop commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

irvingpop Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔴 Hold deployment: Claude of Duty identified 4 items requiring attention

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

irvingpop commented Nov 7, 2026

Uh oh!

irvingpop commented Nov 7, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Nov 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

irvingpop commented Nov 7, 2026 •

edited

Loading

github-actions Bot commented Nov 7, 2026 •

edited

Loading